Skip to content

Allow address-of for type-instantiated generalized let bindings#19948

Merged
T-Gro merged 7 commits into
dotnet:mainfrom
KirtiRamchandani:fix/valuenone-inref-19608
Jul 3, 2026
Merged

Allow address-of for type-instantiated generalized let bindings#19948
T-Gro merged 7 commits into
dotnet:mainfrom
KirtiRamchandani:fix/valuenone-inref-19608

Conversation

@KirtiRamchandani

@KirtiRamchandani KirtiRamchandani commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes FS3236 when taking the address of an untyped ValueNone let binding passed to an inref parameter.

Problem

let x (y: inref<ValueOption<int>>) = ()
let y () =
    let ffff = ValueNone
    x &ffff // FS3236

let fff = ValueOption<int>.ValueNone and let mutable ff = ValueNone already worked.

Fix

In mkExprAddrOfExprAux, treat Expr.App(Expr.Val vref, _, _, [], _) produced by TcVal for generalized let bindings the same as a bare Expr.Val when taking an address.

Testing

  • Added component test Issue 19608 - address of untyped ValueNone compiles
  • Release build succeeds

Fixes #19608

@KirtiRamchandani
Allow address-of for type-instantiated generalized let bindings
f1554c5 
Generalized let-bound values are represented as Expr.App(Expr.Val, tinst, [])
by the type checker. Treat these like bare Val references when taking
their address so `let ffff = ValueNone` works with inref parameters.

Fixes dotnet#19608
@github-actions

github-actions Bot commented Jun 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev

✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@KirtiRamchandani KirtiRamchandani marked this pull request as ready for review June 15, 2026 07:13
@KirtiRamchandani KirtiRamchandani requested a review from a team as a code owner June 15, 2026 07:13
@github-actions github-actions Bot added the ⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen label Jun 15, 2026
@github-actions

This comment has been minimized.

Sign in to view
T-Gro
T-Gro reviewed Jun 15, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Overall: This is a clean, well-targeted fix. The pattern match correctly identifies the expression shape produced by TcVal for generalized let bindings, and the guards (via CanTakeAddressOfImmutableVal checking
ot vref.IsMemberOrModuleBinding) properly restrict the new case to local values only. The logic mirrors the existing Expr.Val case exactly, which is appropriate.

Suggestions

  1. Strengthen test to verify compilation, not just typechecking. The current test does typecheck |> shouldSucceed, but since this fix changes how address expressions are lowered to the typed tree (emitting LAddrOf via mkValAddr), a compile |> shouldSucceed (or even an execution test with compileAndRun) would give stronger confidence that the generated IL is correct. A typechecking-only test won't catch incorrect ldloca emission targeting a val that doesn't have the expected local slot.

  2. Consider adding a negative test for &&ffff (native pointer). The new case calls checkTakeNativeAddress readonly where readonly = true for immutable locals. A test verifying that &&ffff correctly produces an error would validate that branch as well.

Neither of these are blocking — the fix itself is correct and the existing guard conditions are sound.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Jun 15, 2026
@T-Gro T-Gro self-requested a review June 15, 2026 13:04
KirtiRamchandani and others added 2 commits June 15, 2026 13:53
@KirtiRamchandani
Address review feedback and fix CI for dotnet#19608
0249f7a 
- Strengthen regression test to compile (not just typecheck)
- Add negative test for native address-of on immutable binding
- Add release notes entry for FCS 11.0.100
- Apply fantomas formatting to ExprOps pattern match
@T-Gro
Merge branch 'main' into fix/valuenone-inref-19608
65e4a31
T-Gro
T-Gro reviewed Jun 24, 2026
View reviewed changes
Comment thread src/Compiler/TypedTree/TypedTreeOps.ExprOps.fs Outdated
@T-Gro T-Gro marked this pull request as draft July 2, 2026 11:19
@T-Gro

T-Gro commented Jul 2, 2026

Copy link
Copy Markdown
Member

Marking as draft as CI reports error. Pls mark as ready once done.

@KirtiRamchandani KirtiRamchandani marked this pull request as ready for review July 2, 2026 14:25
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Compiler-Output
Affects-Compiler-Output: modifies address-of pattern matching in TypedTreeOps IL generation

Generated by PR Tooling Safety Check · opus46 2.8M · ◷

@KirtiRamchandani

KirtiRamchandani commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@T-Gro Are we good for lgtm now? I am curious the test at: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1491847&view=results said "We stopped hearing from agent NetCore-Public 374...". So the log shows the job was abandoned while installingo rmaybe extracting the .NET SDK. I did not find a compiler/test failure before that. Will it be due to the Azure infrastructure/agent failure, yes?

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jul 3, 2026
@T-Gro T-Gro merged commit 25c6a37 into dotnet:main Jul 3, 2026
50 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in F# Compiler and Tooling Jul 3, 2026
This was referenced Jul 3, 2026
Merged
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@T-Gro T-Gro T-Gro approved these changes

Assignees

No one assigned

Labels

⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen AI-reviewed PR reviewed by AI review council

Projects

F# Compiler and Tooling
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Plain ValueNone binding cannot be passed as inref

2 participants

@KirtiRamchandani @T-Gro